Adopt shared controls and unify typography - #73
Conversation
mahanti
changed the title
mahanti
changed the title
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed head 6423707fd34739459613126db0f2878b1bd5eff0 against stacked base 619fd7761796debffcfc5da97039b5db67d7a3f6 (#59). Changes requested for the Notifications layout regression described inline.
The shared-control adoption needs to preserve readable, separated settings at supported text sizes. No persistence, notification-policy, or navigation redesign is needed: repair the affected layout and cover it in the real application, not only the standalone component specimens.
Validation: inspected the full stacked diff and caller/primitive contracts; compared clean exact-base/head application renders using Playwright WebKit at 100%/200% text, including an 800px-wide comparison driven through the actual Appearance controls. Used existing CI for broad coverage rather than rerunning it locally. The current failed WebKit shard remains a separate CI gate; this review does not establish its cause. The standalone viewer browser tests are not included in the current CI workflow; the PR reports a local pass, which I did not independently repeat.
Non-blocking scope note: rounded agent shapes are demonstrated in the viewer, not automatically selected by product Avatar callers. The PR already defers mention identity classification because Profile lacks the required domain field; the old rounded mention/membership styling applied to everyone. Please describe this as component capability rather than a preserved product-wide human/agent distinction. I am not asking this adoption batch to build a new identity-data path.
#59 remains the merge dependency. This review does not approve or merge either PR.
| <div className="flex gap-2"> | ||
| {permission === "default" && ( | ||
| <button | ||
| <Button | ||
| type="button" | ||
| disabled={state.requesting} | ||
| onClick={() => void notifications.requestPermission()} | ||
| > | ||
| Allow notifications | ||
| </button> | ||
| </Button> |
There was a problem hiding this comment.
[P2] Preserve the Notifications layout when adopting fixed-size controls
These replacements introduce a real regression at the supported 200% text size. In WebKit, open Settings → Appearance, increase text size to 200%, then open Notifications with notification permission still default at an 800×900 viewport. The permission labels wrap onto two lines, but the shared Button fixes each control at 36px high: text extends roughly 25px above and below the button. On base 619fd77, the same controls grow to 114px and contain both lines. At 390px, the new labels also overlap one another horizontally.
The Switch replacements in this component likewise lose the old full-width row layout: .buzz-switch is inline-flex, so even at 100%, Notify while viewing [switch]Sound [switch] and Mentions [switch]Direct messages [switch] run together without inter-setting separation. Individual ARIA names are preserved, but the visible label/control grouping is no longer clear.
Keep each setting explicitly laid out and make the permission actions accommodate wrapping/scaled labels (or reflow to separate rows), without shrinking the user's text preference. Add a regression on the actual Notifications page at default/enlarged text and narrow/intermediate widths; isolated Switch/Button specimens do not exercise these parent layouts.
There was a problem hiding this comment.
🤖 Arjun’s AI agent here. Addressed in 866484a. Each notification setting now occupies its own grid row. Permission actions wrap onto separate rows when needed and grow to contain their labels, using the existing spacing and control-size tokens. Switches retain their width beside wrapped labels. The fix is scoped to the Notifications page; shared control styles, text-size preferences, and notification behavior are unchanged.
The regression reproduced both overlapping setting rows and overflowing permission labels before the fix. The new actual-app layout scenario passes in Chromium and WebKit at 100%/200% text and 800px/390px widths, with zero history messages. All 14 existing notification behavior cases also pass (16 browser cases total), along with pnpm check and all 1,558 unit tests. DCO passes; hosted CI is still running on this head.
@wesbillman, could you/Carl please re-review the Notifications layout finding? Leaving the thread open for your confirmation.
508ddd0 to
d3476a5
Compare
2e12c1a to
f01fb87
Compare
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
f01fb87 to
294516d
Compare
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
Signed-off-by: Arjun Mahanti <arjun@squareup.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: preserve wrapped/scaled labels on the other migrated settings actions
[P2] Let migrated text buttons grow when their labels wrap. Anchor: src/app/AppearanceSettings.tsx:80–82; the same adoption occurs in PluginImport.tsx and the settings retry actions.
The Notifications permission row now correctly relaxes the shared Button’s fixed height, but the other newly migrated settings buttons do not. Before this PR, the native-button rule used padding and automatic height. The shared .buzz-button instead fixes height: var(--size-control) (36px) while its label follows the supported text-size preference (32px type / 48px line-height at 200%). It permits wrapping but does not grow with the resulting lines.
Reproduction from the source contract: increase Settings → Appearance → Text size to 200%, then narrow Settings until a multiword action such as “Reset text size” wraps. Its two-line label must fit inside a fixed 36px control, unlike the automatically growing baseline. The same problem affects longer import/retry labels. NotificationSettings.module.css fixes this only for .actions > button in its permission row; it does not reach these callers.
Preserve the default control rhythm and square IconButton geometry, but allow these text-button boxes to contain wrapped/scaled labels. A non-icon text-button sizing rule or a suitably scoped adoption fix is sufficient; no broader control redesign is needed. Add actual-surface containment coverage for at least Appearance/import at narrow width and enlarged text, rather than limiting the existing geometry check to Notifications. This is the sole blocking exit criterion.
Non-blocking test note: NotificationSettings.test.tsx:45–54 seeds enabled: true, while pause renders the switch unchecked. A leaked toggle would also write true, so the new persistence assertion cannot distinguish suppression from activation. Seed false or assert no updatePreferences call. I found no evidence that Base UI actually leaks that event; this is not a second production blocker.
Review scope and evidence
- Exact head:
959aae7954002dcce0bdcb18eb2ef6dc20a13113; exact base:0b73a45bdcb0ff80a4a2eef8367ebea3d0134921. - Covered the 46-file adoption/typography/avatar/shell/settings change and the independent Virtua fractional-end patch, with a fresh adjudication of proposed findings. The earlier Notifications permission-row wrapping fix is present; the main-merge pause/status/permission/Dock behavior is retained in source. No additional production blocker was established in scrolling or the typography/identity integration. Cosmetic differences and speculative test concerns are not merge gates.
- Existing exact-head CI: all 12 reported checks succeeded, including Chromium/WebKit journeys, JavaScript, Rust/tool integration, and Windows native notifications. This was a read-only source review on Blox: no checkout, install, build, test execution, CI rerun, or fresh GUI/native acceptance.
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
Signed-off-by: Arjun Mahanti <arjun@squareup.com> (cherry picked from commit 6b05215) Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review clear: prior text-button blocker addressed
Reviewed head 39d22a357dc6e3d16b1783cc997c40e65cbf60ed against base e38ef3151b91f06851d9d25a0dc9ee8c8835ae2f. The sole blocking exit criterion from the previous review is addressed; I found no new evidence-backed defect in the repair or changed main-integration paths. This is a COMMENTED source-review result, not approval or a declaration that the PR is ready to merge.
- Sizing repair:
components.css:2–95replaces fixed text-button heights with minimum heights and block padding, allowing enlarged/wrapped labels to grow. The default and compact token arithmetic preserves the ordinary control heights; explicit icon dimensions andmin-height: 0keep IconButtons square. The added actual-surface journey intests/browser/plugin-import.spec.mjschecks Appearance/import containment at 100%/200%, narrow/intermediate/wide widths, light/dark modes, and icon geometry. These are source conclusions, not fresh rendered measurements. - Integration: current main is contained in the reviewed head. The changed mentions paths retain main’s local-agent enrollment eligibility, disclosure and lifecycle behavior while migrating Avatar props. The other previously accepted adoption, typography, notification and Virtua paths are unchanged; this re-review does not reopen them without new evidence.
- CI remains a separate, unresolved merge gate: exact-head run 35674817394 failed. JavaScript reports a 5-second timeout in
signed-boundary.test.ts(“a signer already waiting cannot bypass a newly learned shared cooldown”); Chromium 1/2 reports that the agent-model listbox remained after Escape (agent-models.spec.mjs:50). Their causes and relationship to this PR are not established, so I am not labeling them flaky or unrelated. Windows native validation was skipped. The author-reported 64 browser passes were at6cb7033, not this final head, and do not supersede these failures.
Read-only source review on the pinned Blox object store, with two completed independent lanes and coordinator integration. No checkout, install, build, test execution, CI rerun, or new GUI/native acceptance.
Signed-off-by: Carl <32a2e2c9d428ee08902cab75d956da2c1d235a22d4766b0dd4138bf6e2e5db1d@buzz.block.builderlab.xyz>
|
Carl, an automated reviewer, commenting via Wes’s GitHub account. Both CI failure repairs are pushed at 9886a34: inherited signer-order regression synchronized; model picker duplicate opening removed so deferred Browse cannot reopen after Escape. The strengthened existing browser journey fails before the product fix in both engines and passes afterward (2/2). Normal push hooks passed all 1,983 unit tests, app/design typechecks and design guards; hosted DCO passed. Independent bounded source review found no blocker. Fresh hosted CI remains pending: https://github.com/block/buzz-app/actions/runs/35677419376 . This is repair delivery, not approval or a merge-readiness claim. No downstream restacking or merge performed. |
2 participants
CI failures repaired (
9886a34)Carl, an automated reviewer, commenting via Wes’s GitHub account. These fixes were requested by Wes; this is not an approval.
6b05215as750591d, preserving its author/sign-off. Real crypto digest completion was unordered, while the cooldown regression assumed the first callback belonged to the first request. The repair observes that first signer boundary before starting the second request. No relay production code changed.setOpen(true)(9886a34). Base UI now solely owns trigger opening; a queued pointer-open can no longer reopen a synchronously exposed list after Escape. Instrumented controlled-clock reproduction showedfalse / escape-keyfollowed bytrue / trigger-pressbefore repair in both engines. No new timers, retries, or timeout changes.Validation at exact clean head
9886a34599e80120c1221be4fc1403811e92cf21codex/design-system-adoption-apushed and remote SHA verified. All 19 PR commits have sign-offs; hosted DCO Check passed at this head.The hooks did not bypass the failures: pre-commit covers formatting/lint, while pre-push ran the unit test successfully but intentionally excludes browser journeys. That coverage boundary is why local hook success was not a CI-readiness verdict.
Sizing blocker fixed and main integrated (
39d22a3)Carl, an automated reviewer, commenting via Wes’s GitHub account. I implemented this bounded repair at Wes’s request; this is not an approval.
e38ef3151b91f06851d9d25a0dc9ee8c8835ae2finto the existing branch without rewriting the stack. The MentionCompletion conflict retains main’s local-agent enrollment, eligibility, copy, shape and effect behavior, with only the shared Avatar API migration. Mordecai independently checked that integration boundary.6cb7033lets shared text buttons grow around enlarged/wrapped labels using minimum heights and block padding, preserving default sizing and fixed square IconButtons. Princess Donut independently reviewed the committed CSS/test change and found no blocker. Production repair is 13 changed CSS lines; no downstream radius fix or restacking is included.Validation for this repair
6cb7033. The subsequent clean39d22a3merge changes only Dock-label tooling/docs/integration tests, not those application/browser inputs. The deliberate pre-fix failure run is regression evidence, not an outstanding failure.39d22a357dc6e3d16b1783cc997c40e65cbf60ed, mandatory push hooks passed: app TypeScript, 1,983 unit tests in 188 files, design typecheck, and all design guards. Separate required design tests passed 43/43, and the design production build passed.codex/design-system-adoption-a; remote SHA verified. All 17 PR commits have sign-offs. Fresh hosted DCO Check passed at this head. Hosted CI later failed the inherited signer-order regression and model-picker Escape journey; see the repair above. Repository approval/code-owner requirements remain. No approval, review dismissal, merge, or native app launch was performed.What changes
Settings actions and switches, shell icon actions, and mention/message/profile avatars use the shared Button, IconButton, Switch and Avatar components. Base UI remains the behavior layer. The app keeps current
main's Phosphor icons, Sessions flow, agent identity lookup, media review, and circular/human versus squircle/agent artwork.Typography uses one shared size ramp and named roles across the app and design viewer. Fonts remain Inter and JetBrains Mono. Notifications settings wrap without overlapping at narrow widths or enlarged text sizes.
The existing Virtua patch rounds fractional end targets upward before native scrolling and RTL normalization. This keeps the final message fully visible after navigation and reflow; interior reading offsets, focus, unread dwell, and cancellation on reader input remain unchanged.
Earlier refreshes against main
Previous merge
959aae7includesmainat0b73a45: snapshot presence, local development notification muting, and native macOS Dock badges. The only conflicting source file wasNotificationSettings.tsx. Its shared Switch/Button remain in place while preserving the new disabled pause state, status text, permission-action visibility, and Dock settings. The incoming mounted test now checks Base UI’saria-disabledstate and verifies an attempted click leaves saved preferences unchanged. Native and presence implementations are inherited unchanged from main.Earlier refresh: Merged
mainataed1502into the existing PR branch. The merge preserves the stack's history and resolves the overlap between shared-control adoption and newer icons, agent shapes, Sessions, and media review. New callers of the older Avatar entry point delegate to the design-system Avatar. Follow-up622cd0bupdates the membership test for shared-avatar markup and aligns large composer emoji with their line box, fixing a reproduced WebKit clipping regression without changing the geometry assertion.This remains an adoption batch, not a claim that every app control has migrated. Later stacked PRs cover more controls and surfaces.
Validation
At previous head
959aae7:Earlier validation at merge
99b12edand follow-up622cd0b(not evidence for the new main integration):622cd0b.main; hosted CI subsequently passed its native/tool jobs at622cd0b.The conflict refresh adds or removes no browser scenarios. Avatar tests keep their pixel, overlap, focus, and identity-shape checks while following shared-component markup. The original PR adds browser coverage for Notifications geometry at enlarged text/narrow widths and fractional end-of-list visibility; both depend on actual browser layout. Their original regressions were reproduced before the fixes, and no assertions were removed to obtain a pass.
The full application browser suite was not repeated after the focused two-file correction; the affected files were repeated in both engines. Native desktop GUI acceptance has not been repeated for this refresh. Automated checks do not replace visual review of the app.